refactor(qt): delete the eleven legacy eval runners and verify the frozen inputs - #116
Merged
Merged
Conversation
C6 part two. The unified runner (D5 C4) replaced them, the CLI converged on run-factor-eval, and the verify-only conversion cut the last import edge, so these eleven modules and their ten test files have no remaining reader. Reachability checked in both forms before deleting, not just the static one: no module outside the runners and their own tests names qt.eval_*; every importlib/__import__ site in the repo resolves to factors.* / data.clean.* or imports the `qt` package itself, never a submodule; no module name is built from a string; nothing globs qt/. The only path-based reference anywhere is the R16 guard's own glob over tests/test_eval_*_runner.py, which was written to go vacuous exactly here. Deleted: 11 qt/eval_*.py, 10 tests/test_eval_*_runner.py. NOT deleted and verified present: tests/test_eval_contract_v1.py and tests/test_eval_figures.py, which are not runner tests. peak_ridge_amount_ratio never had a runner test file (catalogue BUG 3) -- a pre-existing gap, not a deletion. Node-ID differential: 2701 -> 2642, removed 59, added 0, and every one of the 59 matches tests/test_eval_*_runner.py::. Zero collateral: nothing outside the deleted files stopped collecting. On what those 59 were: they are GONE WITH THE CAPABILITY, and this commit claims no replacement for them as tests. What the map records -- and what R16 measures -- is that the PROPERTIES they asserted are pinned elsewhere in the new suite: 7 bodies moved verbatim, 50 covered by named new tests, 2 carriers retired with the reason on file (R1). That claim is checked, not asserted: all 56 new-suite node-IDs the map cites still resolve after the deletion, all three family wildcards still have the claimed size, and every citation still resolves to exactly one file. Both R16 runs are now in the map, side by side, because the admission ticket was earned on the pre-deletion tree. The audit script exits 1 afterwards -- its first two sections assert the legacy files exist, which is the pre-deletion framing and cannot hold now. That is NOT a coverage drop; section 3, the one that measures coverage, is clean. The script was deliberately left unfixed so the distinction stays visible, with both raw outputs kept. The durable guard behaves as designed: 6 passed, 1 skipped. Neither the vacuous-by-design check nor the count check goes red, because a guard that failed on the deletion it exists to authorise would be a booby trap. Also fixes the review's NIT-1: hand_anchors_d2's docstring still said the four daily factors "are compared by" the companion. They are not; the companion's rebuild was retired, and those rows now stay pending.
Three read sites opened frozen artifacts with no integrity check at all, while
the directory holding them is demonstrably writable -- one file in it was
overwritten on 2026-07-25. "The harness reconciled against whatever was on disk"
was never hypothetical, so C5's four legs went green against inputs nothing had
authenticated. (They are intact today: verify passes 15/15 panels and 14/14
cells, so no C5 conclusion is in question. Both facts are true and neither alone
is the whole story.)
A/B -- the two frozen-panel reads. `verified_frozen_panel_path` is now THE only
gateway: `frozen_panel_path` is called nowhere else in the module, enforced by
AST, because checking "is this read verified?" per site makes it a per-site
question and the site that forgets is the one that will not announce itself.
The anchors leg's tolerance for an absent panel became `required=False` rather
than a second bare call, which is what collapsed the two callers into one.
Per-panel, not a whole-tree sweep: the harness runs per factor, and verifying
fifteen panels to read one (3.0s vs 0.2s) makes the check something people
switch off, and a check that is disabled because it is slow protects nothing.
The jump fork is followed off the PATH, not re-derived from the factor id: both
trees hold a jump_amount_corr_20.parquet and they are DIFFERENT definitions
(v1.0 untruncated, v1.1 truncated), so re-deriving would eventually drift from
what frozen_panel_path actually picked. Written in a code comment, not only in
the review thread.
C-1 -- the hand-anchor record. It was the one file under refactor_baseline/ that
no manifest covered and the one that actually got overwritten. Its sha256 now
lives in git (docs/factors/d5_hand_anchor_record_manifest.json) while the record
stays gitignored -- the same split as everywhere else, so an overwrite cannot
move its own expectation without appearing in git diff.
Pinned rather than regenerated on purpose: the C5 anchors leg closed 11/11
against THESE bytes, and the stronger provenance form needs a rerun, which would
overwrite them. That would leave a C5 leg verified against a record that no
longer exists.
The manifest states the record's inventory AND the inventory is checked, not
merely written down -- because the pinned record is INCOMPLETE:
daily_engine_compared is 0 rows and cannot be refilled. Pinning an incomplete
record is fine; letting a later reader take "pinned" for "complete" is not. A
test asserts the manifest says so, including that the D2 conclusion itself
survives elsewhere ("88 hand anchors, 0 mismatches", PR #89). The inventory
check also catches what a sha alone cannot: a rerun that drops rows and has its
sha refreshed to match.
D-1 -- the write side. hand_anchor_rows now refuses to overwrite an existing
record without --allow-overwrite, naming the keys this run would drop, computed
by DIFFERENCE against what it produces so a future key needs nobody to remember
a list. The check runs BEFORE the ~7 minutes of hand computation: a refusal that
arrives at the end of a long run teaches people to pass the override by reflex.
PRODUCED_RECORD_KEYS is asserted against the actual payload at write time, so
the set the guard reasons about cannot drift from what is written.
D-3 (carrying the old daily_engine_compared forward) stays rejected and is
recorded as rejected: it would splice a comparison made against older engine
values onto a freshly recomputed frozen14 -- internally inconsistent, worse than
honestly missing. Writing down the tempting option that lost is more useful than
only writing down the one that won.
Two things found while building this, both fixed:
- the CLI shim's `except RecordOverwriteRefused` did not catch. Under `python -m
qt.hand_anchors_d2` the module is loaded twice, as __main__ and as
qt.hand_anchors_d2, so the class raised and the class caught were different
objects and the guard surfaced as a traceback with its handler right there.
- the first mutation aimed at A/B had NO teeth: reverting the read site to the
bare path broke nothing, because no test pinned the wiring -- only the routing.
That is what prompted the single-gateway restructure; the property is now
checkable and the mutation is red. 25/25 mutations have teeth.
C-2 (record the panels_d2 hashes inside the record) and D-4 (move it out of a
directory documented as un-regenerable, which it is not) are registered in the
catalogue for a follow-up that must also re-verify the anchors leg. They are one
piece of work, and this PR's job is deletion.
`artifacts/` -- with a trailing slash -- matches only a directory, so it never
matched the `artifacts` SYMLINK the git worktrees carry (pointing at the main
checkout). That left it untracked-but-addable, and a `git add -A` in this PR duly
committed a machine-specific absolute-path symlink plus two generated phase0
outputs.
This belongs in this PR rather than a later one: the PR exists to make "frozen
bytes stay out of git" true in fact, and the root cause was that the rule was
never correctly expressed. Two lines remove the whole class.
The symlink was caught by
test_frozen_bytes_are_untracked_while_the_manifest_is_tracked, which guards the
split every tamper check here rests on -- if the bytes and their hashes both
lived in git, one commit could move them together and the checks would be
decorative. It fired at exactly the right moment.
`artifacts_local_phase0/` needs its own line: `artifacts` does not match it, and
NO test guards it, so the identical accident there would have been silent. The
guard is deliberately not extended to cover it -- it is not load-bearing for the
frozen design, and widening a guard's range inside a deletion PR is how range
creeps.
Evidence, before and after, with the change asserted to have landed
(.gitignore sha e1f17c45 -> 9791dd9b):
before: git add -A --dry-run -> artifacts
artifacts_local_phase0/logs/run_phase0.log
artifacts_local_phase0/reports/phase0_summary.md
after : git add -A --dry-run -> .gitignore (this change only)
git check-ignore -v -> .gitignore:21:artifacts artifacts
.gitignore:24:artifacts_local_phase0/ artifacts_local_phase0
No tracked path contains "artifacts", so the broader pattern cannot mask
anything already in the repository.
…st that it works Review MEDIUM-1. `load_anchor_rows`'s call to `verify_anchor_record` was pinned by nothing: the reviewer replaced it with `pass` and the whole suite stayed green -- 2661 passed, rc=0, identical to the unmutated tree. This is my own failure form, surviving in the third read site after I fixed it in the other two. The first A/B mutation was empty for exactly this reason: six tests exercised the verifier thoroughly, and none said "the read site must call it". Fixing the panels side by collapsing to a single gateway did nothing for the anchors side, because a lesson applied only where it was learned is not applied. Behavioural, not AST, and the reason is the failure it has to catch: an AST assertion proves the call is written down. A call with the wrong argument, or one whose exception is swallowed, satisfies AST and stops nothing. So the test builds a miniature repo_root, tampers the record AFTER its manifest is written (bytes drift, expectation does not -- the real shape), and demands a refusal. A second mutation proves the distinction is not theoretical: swallowing the exception while keeping the call is now red too, and AST would have passed it. A green control comes first -- an intact record must return its rows -- because "it raised" means nothing from a function that raises at everything. The reviewer made the same point about its own R5: the control must be ACCEPTED first or every subsequent REFUSED is empty. Also pinned: the check runs BEFORE parsing. An unparseable tampered record must fail as an integrity mismatch, not as a JSON error that never names the real problem. Review LOW-1. `top_level_keys` was stated in the manifest and never compared -- INVENTORY_FIELDS held four count-shaped fields. The reviewer added `all_ok_daily: true`, refreshed the sha, and it was ACCEPTED, while the manifest's most prominent prose says precisely that key is absent. So "pinned is not complete" was resting on prose, and prose stops nothing. Counts cannot see a key appear or vanish; the key SET can. It is now compared like every other inventory field. Note this makes the catalogue's existing claim -- that the inventory is machine-checked rather than prose -- true of all five fields, where it had been true of four. Mutations: 28/28 with teeth, three new. #23 is the reviewer's own, reproduced verbatim. NIT-1 (the FAMILY regex not matching a parenthetical) is left alone as ruled: pre-existing, byte-identical to main, and no actual exposure -- both members of that family are cited by name elsewhere and remain covered by the exists-and-is- unique check.
…ally blocks Review finding, and the same defect class this repository keeps catching: a guard whose DECLARED range is wider than what it blocks. Precedent for the fix is `_complete_grid_bars`, which states its exposure per caller rather than asserting a general property. The function docstring promised the inventory catches "a manifest updated to match a new file without anyone noticing what changed inside it (in particular, a daily_engine_compared that silently went to zero)". From the CURRENTLY PINNED record that case cannot occur: the loss already happened, what is pinned is the state after it, and the field is already at zero. Measured at the real pinned shape (70 / 20 / 0 / False) rather than reasoned: untouched -> ACCEPTED (control) rerun, sha NOT refreshed -> REFUSED (the sha catches it) rerun, sha honestly refreshed -> ACCEPTED <- inventory contributes nothing daily_engine_compared 0 -> 20 -> REFUSED <- inventory does bite here A plain rerun keeps every count and the key set identical -- same seed, same stratified selection -- while the values move. So on that path the protection is the human reading the git diff, not this check. Adding top_level_keys did not change this: a rerun does not move the key set either. Both halves are now in the docstring, because either alone misleads: "the inventory is checked" invites more confidence than it earns on the rerun path, and "the inventory is useless" is false in the other direction. Two tests measure the range instead of restating it -- one asserting the LIMITATION (rerun + refreshed sha is accepted) and one the residual teeth (a shape change is refused). The limitation test is deliberately shaped so that strengthening the check later makes it fail: that is the signal to update the RANGE section in the same commit, so the claim cannot drift wider than the code again. A third test pins the docstring itself, whitespace-normalised -- these sentences wrap, and a raw substring match would be testing line breaks rather than the claim (the same wrapped-string trap already hit once in this suite). Mutations 30/30, two new: restoring the unreachable promise, and stating the limitation without saying what covers it instead. Both turn the docstring test red.
… wide Follow-up to 0d91164, prompted by the standing rule that a claim wider OR narrower than the behaviour both count as unfixed. Narrower is a real defect, not pedantry: it invites someone to add a redundant guard for something already covered. The first version said the inventory "bites where counts or keys move". Measured, that understates it: ``all_ok_frozen14`` is a bool verdict flag — neither a row count nor a key — and it IS compared. counts identical, key set identical, all_ok_frozen14 False -> True, sha honestly refreshed -> REFUSED So the range now names all three things the shape comprises: the three counts, the key set, and the verdict flag. And the exemption is restated as what it actually is — an UNCHANGED SHAPE, not the act of rerunning. A rerun today lands in the exemption because same seed gives the same counts and keys and the same inputs give the same flag; a rerun whose flag flipped would be caught. Tests now measure all three directions rather than two: the limitation (unchanged shape plus refreshed sha is accepted), rows gained, and the flag flip — the last with explicit premise assertions that no count and no key moved, so it cannot pass for the wrong reason. The docstring test pins the widened wording, including that it must not read "counts or keys". Mutations 31/31; the new one narrows the claim back and turns the docstring test red.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
C6 的第二个 PR:删除。 改造 PR(#115)把三个重生成工具退役为只读验证;本 PR 删掉它们曾经依赖的 11 个旧 runner,并把校验真正接进 C5 对账 harness 的读入点。决策③"杀第二条因子取数路径"到此收尾。
31 files changed, +1,082 / −9,198删除
qt/eval_*.py+ 10 个tests/test_eval_*_runner.pytests/test_eval_contract_v1.py、tests/test_eval_figures.py可达性含动态形式自查 + 评审独立复核:静态 0 import;
importlib/__import__站点解析到factors.*/data.clean.*或只 importqt包;字符串拼名只有factors.compute.minute.{stem};qt/的 glob 只匹配intraday*.py/exec_basis*.py。node-ID:removed 59,落在被删文件之外 0 条
按文件分布
4+4+4+4+4+10+14+4+7+4,评审用独立导出树重采并与映射表独立 grep 的口径逐格吻合。R16 覆盖数非降(删除的准入证,删前删后各跑一次)
审计脚本删后 rc=1 只来自它 [1]/[2] 的删前口径("旧套件应有 10 文件 59 条"),判定覆盖的 [3] 是 0 problems。没有把脚本改成"删完也 rc=0"——那会掩盖这个区分;两份原始输出留档。评审亲自跑过并认同该处置。
把校验接进 C5 harness 的三个读入点
改造 PR 只登记了这三处裸读,本 PR 接线:
run_panels_moderun_anchors_mode--mode anchors单跑时唯一的冻结面板读入点)load_anchor_rowshand_anchors_d2.json面板侧:
verified_frozen_panel_path成为唯一网关(frozen_panel_path全模块只在网关内被调用,AST 强制)。按因子校验而非整棵扫(0.2s vs 3.0s)——为读 1 个而验 15 个,会把校验变成大家去关掉的东西。jump 分叉是承重的:两棵树都有
jump_amount_corr_20.parquet而它们是两个不同定义(v1.0 未截断 / v1.1 已截断,canonicalb6359f12…vsfafda485…),所以路由按路径读、不从 factor id 重推;评审实测拿 D1 的期望去核 PR-C 字节 → 正确拒绝。记录侧(
hand_anchors_d2.json是 7/113 个不被任何哈希覆盖的文件之一):新增 git 里的docs/factors/d5_hand_anchor_record_manifest.json钉住 sha + 清单,读前核。那份 manifest 的射程:只写它真能挡的
被 pin 的记录是 70 / 20 / 0 / False——07-25 那次事故之后的状态。射程经两轮收窄才到位:
daily_engine_compared悄悄归零」。但那个 case 在当前 pin 状态上已不可能再发生——损失已经发生,被 pin 的是损失之后的状态,那个字段已经在地板上。all_ok_frozen14是布尔判定标志、既非计数也非键,却一直在被比较。(最终射程 = 形状(三个计数 + 键集 + 判定标志),豁免重述为**"形状未变"而不是"重跑这个动作"。评审在真实 pin 形态上跑了六条带**,6/6 与措辞一致:
并且实测了动机形态的真实方向:若 manifest 当初 pin 在记录还有 20 行 compared 时、记录随后丢掉它们 + 刷新 sha → REFUSED。所以"若当初 pin 在事故之前,这条检查会抓住它"是跑出来的,不是推理。
那条"承认限制"的测试被刻意做成会在检查被加强时变红——评审真的加了第 6 个 inventory 字段验证:它红了。⇒ 声明不可能再次比代码宽,这个自我保护机制不是装饰。
hand_anchor_rows不再静默覆盖冻结记录目标已存在即拒绝,除非显式
--allow-overwrite;拒绝信息按差集算出"这次跑不会重新产生的键"(不靠谁维护清单),并说明覆盖后须同 commit 更新 manifest。检查前移到 ~7 分钟计算之前——在长跑结束才说"拒绝",教出来的是反射性加 override。明确否决 D-3(读回旧文件、carry forward 自己不产生的键):那会把对着旧引擎值算出的比较结果与刚重算的值拼进同一份文件 ⇒ 内部不自洽的记录,比诚实的缺失更糟。因为它很诱人,所以写进案卷。
hand_anchor_rows的 print 曾在教用户跑一条已退役的命令——扫全仓后发现同族共 4 处,指针 author-once 收敛为ENGINE_COMPARISON_POINTER,printer 只组合不复述。hand_anchors_engine_values→qt.panel_freeze→data.clean.schema,而手算侧绝不能载入引擎(评审实测import qt.hand_anchors_d2载入 756 个模块,factors.*/data.clean.*恰好 0 个)。用 AST import 检查钉住。.gitignore:规则终于表达了它一直想表达的意思git add -A曾把一个指向本机绝对路径的artifacts符号链接提交进来——被test_frozen_bytes_are_untracked_while_the_manifest_is_tracked抓住,那条测试守的正是整个校验设计赖以成立的 git/gitignore 分家。根因:.gitignore的artifacts/带斜杠,既不匹配符号链接也不匹配artifacts_local_phase0/(后者无人守)。改为artifacts(无斜杠)+ 新增artifacts_local_phase0/。代价已写进规则旁:那两个路径从此在git status里不可见。评审揪出的、最值得记的一条
"只钉函数、没钉接线"这个洞,在三个读入点里活下来一个。 面板侧修好后有两道守卫,而记录侧六条测试把
verify_anchor_record这个函数测得很扎实、却没有一条说load_anchor_rows必须调用它 ⇒ 评审把那行换成pass,全量套件 2661 passed / rc=0,与未变异树逐数相同。实现方的话:「一个只在学到它的地方被应用的教训,等于没被应用。」
修法用行为测试承重而非 AST,并把理由做成 mutation #24:保留调用、只吞掉异常 ⇒ AST 通过、行为测试红。这把"优先行为"从论断变成了可复跑的性质。
Gates(lead 独立重跑,未采信 subagent 自报)
pytest -p no:warnings2669 passed + 1 skipped ·ruffclean · phase0 锚 ic 0.9600 / annual 0.8408 ·validate-config32/32 ·exec_baseline_freeze --verify77/77 @ 45c14aa · 三个重生成入口 rc=1(loud) · 两个--verifyOK · 旧 CLI 名 rc=1 + 迁移提示 / 新名 rc=0 ·git add -A --dry-run无可暂存项 · secret 0 · 冻结基线自 2026-07-25 起 0 写入实现方 mutation 31/31 有牙;评审两轮各自独立写 mutation 复现关键项;真实数据 mutation 全程在
cp -r副本上,两方各自前后快照证明冻结基线 0 写入。过程记录:同一个协议洞复发三次
评审两次发现树在它测量期间被改动(一次因 lead 收到主报告就派返工、而评审还有追加项未完成;一次因实现方声明停手后自审出真缺陷直接改了)。两次都是"与预期矛盾的观察"抓住的,不是测试。 三方各补一条才闭合:lead 等全部条目交付且明确收工才派返工;评审每份报告显式声明是否收工;实现方声明停手后非经指派不得恢复工作——哪怕是自己发现的真缺陷,也先报告再动手。
两次污染都被逐条厘清射程而非笼统作废(受污染的用
git archive导出干净树重测),所以没有一轮需要重来。